-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(v2): Added Lighthouse CI to PR checks #3761
Conversation
Deploy preview for docusaurus-2 ready! Built without sensitive environment variables with commit b407ab8 |
@@ -64,6 +64,8 @@ function DocSidebarItemCategory({ | |||
return isActive ? false : item.collapsed; | |||
}); | |||
|
|||
const [enableCSSStyles, setEnableCSSStyles] = useState(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi
You probably messed up with your Git workflow, because it's not supposed to be there 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was 2 am IST. I will fix it today, that is why I didn't message for a review 😅
@@ -124,8 +124,7 @@ export default ({ | |||
if (metastring && codeBlockTitleRegex.test(metastring)) { | |||
// Tested above | |||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
codeBlockTitle = metastring | |||
.match(codeBlockTitleRegex)![1]; | |||
codeBlockTitle = metastring.match(codeBlockTitleRegex)![1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have this code formatting fixed on master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just need to rebase, will do that when I make the other required changes. Sorry for the confusion
Thanks Do you know how I can validate this PR works as intended? Will the comment only work after a merge? |
We definitely need to create our own config for Lighthouse ( |
@slorber The stats will be provided on each PR. To check them for now you can go inside 'details' in the Github action and check for the Audit tab |
@slorber Since you gave the go ahead to upload the Lighthouse stats on a public server, I can add a comment that adds the URL to the uploaded stats just like the Netlify bot. I think that'll be good? |
@lex111 Do the score check only need to be applied to the PWA or both online and offline versions? |
I think checking one URL (with |
removed wait for Netlify action corrected Netlify deploy urls corrected sitename corrected sitename
b5937ac
to
3873153
Compare
@lex111 Netlify does not generate an offline preview mode so if I just add this PWA then it will check the live version of it and not the version that is generated with the new changes. So I don't think we should add it. If you had anything else in mind, do let me know. As far as the config file is needed, it comes out of the box with the action and a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPD: haven't seen the previous comment, if none of above can be done then LGTM 👍 from my part.
Timed out waiting for start_url (https://deploy-preview-3761--docusaurus-2.netlify.app/classic/index.html) to respond
As I wrote earlier, the URL must be used with offline mode enabled, then this error will not occur.
Also it is better for us to use the config file for Lighthouse, then we can exclude some checks that only distract attention:
- Page is blocked from indexing
- robots.txt is not valid
- Document does not have a valid
These checks will never pass on Netlify deploys, so we can turn them off. As a result, we don't see real SEO metrics.
@lex111 Added a
Unfortunately couldn't find an audit for page is blocked from indexing in the lighthouse config documentation. If you can find it then let me know, I'll add it. |
Maybe |
@slorber The comment is ready I think it is just failing because you'll have to approve the action / PR. Here's a screenshot of the generated comment. I think this is what you wanted. Let me know if any changes are required. |
@sarthakkundra nice work, but can you please add |
@lex111 Done! :) |
@sarthakkundra thanks, but it doesn't seem to work :( And if you try to add just |
@lex111 Doesn't seem to work :/ |
So, I understand, please add |
@lex111 Have a look |
Excellent! :) |
Thanks, LGTM, but we'll see after merge if it works fine or not ;) |
Hahaha. Sure. Let me know if any changes are required :) |
@sarthakkundra it seems to work fine, FYI had to change this setting so that it works in forked repos too (#3846) Was wondering if we can try to make the Lighthouse score more reliable. I think Netlify needs some warm up, wonder if it could get a more. predictable score by running Lighthouse twice on it (or more?), and uploading only the latest results, and uploading only the last result once the CDN caches are hotter? Are there other settings we can use to make this score more reliable? (I don't know, maybe can we disable TTFB scores and everything that relates to network speed etc) |
@slorber We can definitely make Lighthouse run Maybe you can open an issue and I'll look into the possible ways to make the scores better and make a PR? I'll change the number of time it runs and probably disable tests that are giving a false output / not applicable on Netlify deploys. |
Motivation
Added Lighthouse CI to integrate finally and create a complete Build bot to analyse stats changed with every new pull request.
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
A new comment action is added.
Related PRs
#3744